fix(sandbox): docker sandbox native file transfer + reject truncated downloads (#2618) - #2923
Open
larry-zy wants to merge 3 commits into
Open
fix(sandbox): docker sandbox native file transfer + reject truncated downloads (#2618)#2923larry-zy wants to merge 3 commits into
larry-zy wants to merge 3 commits into
Conversation
…downloads (agentscope-ai#2618) Implement SandboxFileTransfer for DockerSandbox via docker cp (host temp file <=> container), so upload/download never pass file bytes through docker exec argv (base64 E2BIG for large files) nor the 512KB stdout truncation cap. - DockerSandbox: supportsFileTransfer/uploadFile/downloadFile with workspace-constrained path validation (rejects root, traversal, outside-workspace, blank/null); truncation-free docker cp round trip. - SandboxBackedFilesystem: fail downloads whose exec fallback output was truncated instead of silently returning partial base64. - Tests: unit coverage for path/boundary/container edge cases; opt-in (-Ddocker.it=true) real round-trip integration tests against a live Docker daemon (>1MB binary bit-for-bit, spaces/quotes, relative/absolute, workspace root '/', missing-file non-zero exit, temp-file cleanup).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
larry-zy
changed the title
…daemon Add an overridable runDockerCliBlocking seam and a recording DockerSandbox subclass so unit tests drive the uploadFile/downloadFile temp-file plumbing (mkdir, docker cp, temp write/read, cleanup on success and cp failure) without requiring -Ddocker.it=true. Raises patch coverage past the codecov gate; the docker cp execution paths were previously exercised only by the opt-in integration tests, which CI does not run.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #2618.
SandboxBackedFilesystem.uploadFilesfails deterministically on large files. Three compounding causes:SandboxFileTransferinterface existed but no sandbox implemented it, so every upload fell back to the base64-in-argv path.ARG_MAXexceeded —DockerSandboxpasses that whole command as a singledocker execargv element, blowing past the OS argument limit (~1MB on macOS, 128KB per-arg on Linux).Real-world trigger:
SessionTree.mirrorToFilesystemmirroring 50+ messages produces JSONL >768KB; base64 inflates it ~33% and the upload permanently fails.Fix (Option A from the issue)
DockerSandbox implements SandboxFileTransfer—supportsFileTransfer/uploadFile/downloadFiletransfer bytes via a host temp file +docker cpround trip. File content never passes throughdocker execargv, so there is noARG_MAXlimit and no 512KB stdout truncation cap...traversal, outside-workspace, and blank/null paths.SandboxBackedFilesystem— when a download falls back toexecand the sandbox truncates stdout, returnFileDownloadResponse.fail(...)instead of silently handing back partial base64 (avoids silent data corruption).The dispatch logic in
uploadFiles/downloadFiles(active instanceof SandboxFileTransfer) already existed; it just had no implementation to reach. Docker now actually hits the fast path.Tests
-Ddocker.it=true, 9 passing against a live Docker daemon): >1MB binary bit-for-bit round trip, paths with spaces/quotes, relative/absolute paths, workspace root/, missing-file non-zero exit, temp-file cleanup.All green locally (unit + Docker integration).